Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Easier Settings for Tray publisher addon #44

Merged
merged 14 commits into from
Dec 3, 2024
Merged

Easier Settings for Tray publisher addon #44

merged 14 commits into from
Dec 3, 2024

Conversation

MustafaJafar
Copy link
Contributor

@MustafaJafar MustafaJafar commented Nov 27, 2024 •
edited
Loading

Changelog Description

resolve #43

This PR only changes the settings UI.

  1. Make items of Simple creators easier to read.
    by un expanding the item groups.
    image
  2. Add various enhancements to IngestCSV settings
    • Columns settings are expanded, making it easier to have a quick look through the whole section
      image
    • Doing the same with representations
      image
    • Rearrange the Folder creation config setting and change the layout of folder types to compact (so it looks like a table of regexes)
      image

Testing notes:

  1. Everything should work as before.

@MustafaJafar MustafaJafar added the type: enhancement Improvement of existing functionality or minor addition label Nov 27, 2024
@MustafaJafar MustafaJafar self-assigned this Nov 27, 2024
@MustafaJafar MustafaJafar marked this pull request as draft November 27, 2024 20:47
@MustafaJafar MustafaJafar marked this pull request as ready for review November 27, 2024 23:04
@MustafaJafar MustafaJafar marked this pull request as draft November 27, 2024 23:27
@iLLiCiTiT
Copy link
Member

This PR changig a lot of things at the same time.

I don't think we should put simple and editorial creators under create TBH. That's my point of view, so we'd need some other person to decide that...

@MustafaJafar MustafaJafar marked this pull request as ready for review November 28, 2024 13:36
robin-ynput
robin-ynput approved these changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@robin-ynput robin-ynput left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate most of this can be seen as personal preferences.

But I'd say I'm happy with those changes:
🟢 Ingest CSV column expanded
🟢 Ingest CSV representation expanded

Slightly less sure about those ones:
🟠 Folder/Task Regexes order switch - current order makes more sense to me
🟠 Grouping settings under Create - looks fine on paper but I haven't played much with those option so it's hard to give a great feedback here

I'm giving an approval here to not slow things down, I feel once discussions will happen I this will just be matter of finding a proper compromise.

server/settings/creator_plugins.py Show resolved Hide resolved
@MustafaJafar
Copy link
Contributor Author

🟠 Folder/Task Regexes order switch - current order makes more sense to me

@robin-ynput , I admit the changes mostly came from my personal preference.
But, honestly, the original settings are uncomfortable to my eyes 😅, they go like:

item
expandable folder
item
expandable folder

image

I've here few suggestions, please let me know which one do you prefer:

Current status of this PR Alternative suggestion
image image

@robin-ynput
Copy link
Contributor

robin-ynput commented Dec 2, 2024 •
edited
Loading

Important thing imo is really to keep Folder/Task grouping. I think the alternative suggestion is great !!

@MustafaJafar
Copy link
Contributor Author

should this PR get a bump minor label?

@iLLiCiTiT
Copy link
Member

iLLiCiTiT commented Dec 3, 2024 •
edited
Loading

should this PR get a bump minor label?

You should split the PR to multiple PRs. There are different major changes, some of them cannot be merged and some of them should be approved by different people.

I'm still against moving simple and editorial creators under create section. Using name in settings model cannot be used the way you've changed it. Changing order of attributes is something that should be reviewed on it's own without the other changes.

@MustafaJafar
Copy link
Contributor Author

@iLLiCiTiT @robin-ynput
Thank you for your input. I've reverted some changes. this PR now only changes the UI/layout of the settings.

@MustafaJafar MustafaJafar merged commit 19cef9b into develop Dec 3, 2024
3 checks passed
@MustafaJafar MustafaJafar deleted the 43-easier-settings-for-tray-publisher-addon branch December 3, 2024 23:50
@MustafaJafar MustafaJafar mentioned this pull request Dec 4, 2024
Open
iLLiCiTiT
iLLiCiTiT reviewed Dec 4, 2024
View reviewed changes
server/settings/simple_creators.py
@@ -2,7 +2,7 @@


class SimpleCreatorPlugin(BaseSettingsModel):
_layout = "expanded"
Copy link
Member

@iLLiCiTiT iLLiCiTiT Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really should stay there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, Expanded layout in this setting is very confusing. it's very hard for me to go through it.
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried updating the setting group name with label but that was not recommended because admins are allowed to leave the label blank.

Also, I tried updating the setting group name with product_type but AYON server ignored the name in favor of the label.
https://discord.com/channels/517362899170230292/773127690453516298/1313448327903445012

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know you've tried to change, but then reverted, so this should be reverted too. It was bad before, now it is worse...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll create another PR this afternoon.

This was referenced Dec 9, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iLLiCiTiT iLLiCiTiT iLLiCiTiT left review comments

@robin-ynput robin-ynput robin-ynput approved these changes

@BigRoy BigRoy Awaiting requested review from BigRoy

@jakubjezek001 jakubjezek001 Awaiting requested review from jakubjezek001

Assignees

@MustafaJafar MustafaJafar

Labels
type: enhancement Improvement of existing functionality or minor addition
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Easier Settings for Tray publisher addon
3 participants
@MustafaJafar @iLLiCiTiT @robin-ynput